-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make tidb adaptor in errors. #2
Conversation
format_test.go
Outdated
testGenericRecursive(t, err, want, wrappers, 3) | ||
} | ||
} | ||
//func TestFormatGeneric(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why comment out this test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because we change the logic, N x N combination testcase is no longer suitable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use a block comment /* */
to reduce the changed lines.
errors.go
Outdated
@@ -115,6 +115,10 @@ func Errorf(format string, args ...interface{}) error { | |||
} | |||
} | |||
|
|||
type withStackAware interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not hasStackAware
?
errors.go
Outdated
@@ -119,6 +119,14 @@ type withStackAware interface { | |||
hasStack() bool | |||
} | |||
|
|||
func hasStack(err error) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to errHasStack
to avoid name conflict?
LGTM |
This is done by using the new AddStack function instead of WithStack. There is also a StackError utility function exposed.
This is roughly (no generics) the Is function in the go 2 proposal
I have a PR to this branch: https://github.com/lysu/errors/pull/2 |
LGTM |
I don't have write access. |
errors.go
Outdated
// StackTrace() errors.StackTrace | ||
// } | ||
// | ||
// invoked. This information can be retrieved with the StackTracer interface that returns a StackTrace. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that all the comment in this file is aligned to 80 columns, should we also keep the format?
errors.go
Outdated
// HasStack checks for this marker first. | ||
// Annotate/Wrap and Annotatef/Wrapf will produce this marker. | ||
type StackTraceAware interface { | ||
hasStack() bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be upper-case to HasStack
?
group.go
Outdated
|
||
// WalkDeep does a depth-first traversal of all errors. | ||
// Any ErrorGroup is traversed (after going deep). | ||
// The visitor function can return false to end the traversal early |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from the code logic, in order to terminate the traversal the visitor must return true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤣 it's bug and never got to siblings, I try modify it and add a testcase. PTAL
group.go
Outdated
} | ||
|
||
// Go wide | ||
if hasGroup, ok := err.(ErrorGroup); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/hasGroup/group/?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Well, I don't have write access too. |
group.go
Outdated
func WalkDeep(err error, visitor func(err error) bool) bool { | ||
// The visitor function can return true to end the traversal early | ||
// In that case, WalkDeep will return true, otherwise false. | ||
func WalkDeep(err error, visitor func(err error) bool) (done bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to avoid named parameters since new gophers are not necessarily familiar with them and they create stateful variables. I like the concept of trying to give a name to return types, it would just be nice if they didn't come with the extra ability. If keeping this variable I would also name this "early" rather than "done": from the visitor function perspective it is "done", but from the perspective of calling "WalkDeep", it is always done when it returns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree~ I rename them.
ps: I am not often using named parameters, too..but preivous days, I read this golang/go#20859 issue make me changed 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting
I did already approve the PR, which in my understanding implies "LGTM". Thanks for the follow up fixes! |
friendly ping @coocood |
What have you changed? (mandatory)
adaptor pkg/errors to tidb/tidb-operator, mainly worked by @gregwebs
ref issue
#1
https://github.com/lysu/errors/pull/2
pingcap/tidb-operator#80
pingcap/tidb#7151
What is the type of the changes? (mandatory)
How has this PR been tested? (mandatory)
Does this PR affect documentation (docs/docs-cn) update? (mandatory)
no
Does this PR affect tidb-ansible update? (mandatory)
no
Does this PR need to be added to the release notes? (mandatory)
no
Refer to a related PR or issue link (optional)
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)